-
Notifications
You must be signed in to change notification settings - Fork 25
fix(opentelemetry): attributes missing on validation errors #1530
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
529318e
to
627a488
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes missing attributes on validation errors in OpenTelemetry spans by restructuring how GraphQL operation attributes are set during different phases of query processing.
Key changes:
- Refactored attribute setting to ensure operation metadata is available during validation errors
- Added proper error code collection and attribute setting for validation spans
- Enhanced test coverage for validation error scenarios with Hive tracing
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
packages/plugins/opentelemetry/tests/utils.ts | Updates test utilities to support custom tracer providers and switches from proxy to supergraph configuration |
packages/plugins/opentelemetry/tests/useOpenTelemetry.spec.ts | Adds validation error test case and updates existing tests for new supergraph setup |
packages/plugins/opentelemetry/src/spans.ts | Refactors attribute setting functions and adds operation caching mechanism |
packages/plugins/opentelemetry/src/plugin.ts | Updates plugin hooks to properly set attributes during validation and execution phases |
e2e/opentelemetry/opentelemetry.e2e.ts | Adds focused end-to-end test for validation error telemetry |
e2e/opentelemetry/gateway.config.ts | Adds alternative configuration setup for testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
operationSpan.span.attributes['graphql.operation.name'] === 'test'; | ||
operationSpan.span.attributes['graphql.operation.type'] === 'query'; | ||
operationSpan.span.attributes['hive.graphql.error.count'] === 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines are using assignment operators (===) instead of assertions. They should use expect() assertions to actually test the values.
operationSpan.span.attributes['graphql.operation.name'] === 'test'; | |
operationSpan.span.attributes['graphql.operation.type'] === 'query'; | |
operationSpan.span.attributes['hive.graphql.error.count'] === 1; | |
expect(operationSpan.span.attributes['graphql.operation.name']).toBe('test'); | |
expect(operationSpan.span.attributes['graphql.operation.type']).toBe('query'); | |
expect(operationSpan.span.attributes['hive.graphql.error.count']).toBe(1); |
Copilot uses AI. Check for mistakes.
|
||
const codes = []; | ||
for (const error of result) { | ||
if (error.extensions.code) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential runtime error if error.extensions
is undefined. Should check if error.extensions
exists before accessing the code
property.
if (error.extensions.code) { | |
if (error.extensions && error.extensions.code) { |
Copilot uses AI. Check for mistakes.
document, | ||
operationName || undefined, | ||
); | ||
} catch {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty catch block suppresses all errors silently. Consider logging the error or adding a comment explaining why errors should be ignored here.
} catch {} | |
} catch (err) { | |
// Failed to get operation AST from document; log error for debugging. | |
console.error('Error in getOperationFromDocument:', err); | |
} |
Copilot uses AI. Check for mistakes.
import { | ||
getOperationASTFromDocument, | ||
isAsyncIterable, | ||
memoize2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The memoize2
import is added but never used in this file. This unused import should be removed.
memoize2, |
Copilot uses AI. Check for mistakes.
🚀 Snapshot Release (
|
Package | Version | Info |
---|---|---|
@graphql-hive/gateway |
2.1.10-alpha-1840765d92a74f7cc279061328104c5ea7b4df49 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/nestjs |
2.0.15-alpha-1840765d92a74f7cc279061328104c5ea7b4df49 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/plugin-opentelemetry |
1.0.12-alpha-1840765d92a74f7cc279061328104c5ea7b4df49 |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-prometheus |
2.0.13-alpha-1840765d92a74f7cc279061328104c5ea7b4df49 |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway-runtime |
2.1.9-alpha-1840765d92a74f7cc279061328104c5ea7b4df49 |
npm ↗︎ unpkg ↗︎ |
1a5b4c6
to
eab949a
Compare
🚀 Snapshot Release (Node Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared
|
🚀 Snapshot Release (Bun Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works!
d157565
to
1840765
Compare
No description provided.